🛡️ Fix: Path System Array Index Out of Bounds Vulnerability#193
🛡️ Fix: Path System Array Index Out of Bounds Vulnerability#193sotoJ24 wants to merge 4 commits intoSunsetLabs-Game:mainfrom
Conversation
WalkthroughAdds safe, non-panicking path access and progression APIs: introduces Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller as Game Logic
participant PS as PathSystem
participant Data as Path Data
Caller->>PS: advance_enemy_position(enemy, path_id, index)
PS->>PS: is_valid_path(path_id)?
alt Invalid path
PS-->>Caller: return enemy unchanged
else Valid path
PS->>PS: is_valid_index(path_id,index)?
alt Index invalid or completed
PS-->>Caller: return enemy unchanged
else Valid and not completed
PS->>Data: get_path_data(path_id)
Data-->>PS: Some(Span)
PS->>PS: get_step_from_array(span, index+1)
PS-->>Caller: Updated enemy (coords, index+1)
end
end
sequenceDiagram
autonumber
actor Caller as External
participant PS as PathSystem
Caller->>PS: get_path_step_result(path_id, index)
PS->>PS: validate path_id
alt Invalid path_id
PS-->>Caller: PathResult::InvalidPath
else validate index
alt Index out of bounds
PS-->>Caller: PathResult::IndexOutOfBounds
else path already completed
PS-->>Caller: PathResult::PathCompleted
else OK
PS-->>Caller: PathResult::Success(x,y)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Flag potential breaking changes that are not documented:
1. Identify changes to public APIs/exports, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints (including removed/renamed items and changes to types, required params, return values, defaults, or behavior).
2. Ignore purely internal/private changes (e.g., code not exported from package entry points or marked internal).
3. Verify documentation exists: a "Breaking Change" section in the PR description and updates to CHANGELOG.md.Please share your feedback with us on this Discord post. ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
sotoJ24
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contract/src/systems/game.cairo (1)
303-349: Fix unsafe Span return from locally-allocated ArrayThe current implementation of
get_path_datareturns anOption<Span<(u32, u32)>>where eachSpanpoints into anArray<T>that was allocated inside the callee. In Cairo v1,Spanis a non-owning view and the backingArrayis dropped when the function returns—making the returnedSpana dangling reference. This must be corrected to avoid undefined behavior.• Location:
contract/src/systems/game.cairo, lines 303–349
• Issue: Returning onlySpan<T>does not transfer ownership of the underlyingArray<T>to the caller.
• Remedy: Return the owningArray<(u32, u32)>(or another owning container) so that its lifetime outlives the function, then the caller can derive aSpanas needed.Suggested fix (one of two approaches):
Return the Array itself
Change the signature to returnOption<Array<(u32, u32)>>and update call sites to derive a span:- fn get_path_data(path_id: u64) -> Option<Span<(u32, u32)>> { + fn get_path_data(path_id: u64) -> Option<Array<(u32, u32)>> { match path_id { 0 => { let path_0_steps = array![ (0_u32, 5_u32), (1_u32, 5_u32), (2_u32, 5_u32), (3_u32, 5_u32), (4_u32, 5_u32), (5_u32, 5_u32) ]; - Option::Some(path_0_steps.span()) + Option::Some(path_0_steps) }, 1 => { let path_1_steps = array![ /* … */ ]; - Option::Some(path_1_steps.span()) + Option::Some(path_1_steps) }, 2 => { let path_2_steps = array![ /* … */ ]; - Option::Some(path_2_steps.span()) + Option::Some(path_2_steps) }, _ => Option::None, } }Then callers can do:
let steps_arr = get_path_data(path_id)?; let steps_span = steps_arr.span(); let point = steps_span[idx];Move static path data into module-level storage
If the set of paths is fixed, define them once as global constants or in a lazily-initialized static, avoiding per-call allocation:const PATH_0: Array<(u32, u32)> = array![(0,5), (1,5), …]; /* similarly for PATH_1, PATH_2 */ fn get_path_data(path_id: u64) -> Option<Span<(u32, u32)>> { match path_id { 0 => Some(PATH_0.span()), 1 => Some(PATH_1.span()), 2 => Some(PATH_2.span()), _ => None, } }Either approach ensures the backing buffer outlives the function call, eliminating dangling spans.
🧹 Nitpick comments (7)
contract/src/systems/game.cairo (7)
212-220: PathCompleted is never produced; wire it into callers or remove itYou introduced PathResult::PathCompleted but no code path currently returns it. Prefer to return PathCompleted from get_path_step_result when index == path_length to distinguish "end-of-path" from generic OOB, or drop the variant to avoid dead code.
Would you like me to update get_path_step_result and add tests that assert PathCompleted is returned at exactly index == length?
224-231: get_path_step: LGTM; consider early length check to avoid building arraysThe implementation is correct and panic-free. Minor micro-optimization: check length first to skip array materialization when the index is obviously invalid.
Apply this diff if you want the early-exit:
- fn get_path_step(path_id: u64, index: u32) -> Option<(u32, u32)> { - // Validate path_id first - let path_data = Self::get_path_data(path_id); - match path_data { - Option::None => Option::None, - Option::Some(steps) => Self::get_step_from_array(steps, index), - } - } + fn get_path_step(path_id: u64, index: u32) -> Option<(u32, u32)> { + match Self::get_path_length(path_id) { + Option::None => Option::None, + Option::Some(len) => { + if index >= len { + Option::None + } else { + match Self::get_path_data(path_id) { + Option::None => Option::None, + Option::Some(steps) => Self::get_step_from_array(steps, index), + } + } + } + } + }
233-247: Return PathCompleted at index == length and reserve IndexOutOfBounds for index > lengthThis makes PathResult exhaustive and semantically precise, and avoids constructing steps when the index is already known to be terminal or invalid.
Apply this diff:
- fn get_path_step_result(path_id: u64, index: u32) -> PathResult { - // Validate path_id first - let path_data = Self::get_path_data(path_id); - match path_data { - Option::None => PathResult::InvalidPath, - Option::Some(steps) => { - match Self::get_step_from_array(steps, index) { - Option::None => PathResult::IndexOutOfBounds, - Option::Some(coords) => PathResult::Success(coords), - } - } - } - } + fn get_path_step_result(path_id: u64, index: u32) -> PathResult { + match Self::get_path_length(path_id) { + Option::None => PathResult::InvalidPath, + Option::Some(len) => { + if index == len { + PathResult::PathCompleted + } else if index > len { + PathResult::IndexOutOfBounds + } else { + match Self::get_path_data(path_id) { + Option::None => PathResult::InvalidPath, // defensive, should not happen + Option::Some(steps) => match Self::get_step_from_array(steps, index) { + Option::None => PathResult::IndexOutOfBounds, // defensive + Option::Some(coords) => PathResult::Success(coords), + }, + } + } + } + } + }
248-276: Behavior is safe and non-panicking; consider emitting lightweight events or returning the new indexThe off-by-one logic using next_index and the early completion check looks correct and prevents OOB. If observability is desired (PR description mentions events), consider emitting an event on success/failure or returning (Enemy, Option) to propagate the next index to the caller without extra storage reads.
Additional code (outside this hunk) to define minimal events:
// Add near other events in this module #[event] pub enum PathEvent { Advanced: (enemy_id: u64, index: u32, x: u32, y: u32), AdvanceIgnored: (enemy_id: u64, reason: felt252, index: u32), }And then inside this function (illustrative only; adjust fields as available):
// on success emit PathEvent::Advanced(enemy_id: enemy.id, index: next_index, x: next_x, y: next_y); // on invalid path/index emit PathEvent::AdvanceIgnored(enemy_id: enemy.id, reason: 'invalid_path_or_index', index: next_index);PR description states “Added event emissions,” but this file doesn’t emit any. Are events introduced elsewhere in this PR? If not, should we include the above minimal events here?
278-294: Avoid duplication: reuse get_path_lengthPath lengths are hardcoded here and in get_path_length. Collapse to a single source of truth.
Apply this diff:
- fn is_path_completed_safe(path_id: u64, index: u32) -> Option<bool> { - // Get the path length for each predefined path - let path_length = match path_id { - 0 => Option::Some(6_u32), // Path 0 has 6 steps (indices 0-5) - 1 => Option::Some(7_u32), // Path 1 has 7 steps (indices 0-6) - 2 => Option::Some(9_u32), // Path 2 has 9 steps (indices 0-8) - _ => Option::None, - }; - - match path_length { - Option::None => Option::None, - Option::Some(length) => Option::Some(index >= length), - } - } + fn is_path_completed_safe(path_id: u64, index: u32) -> Option<bool> { + match Self::get_path_length(path_id) { + Option::None => Option::None, + Option::Some(length) => Option::Some(index >= length), + } + }
360-368: get_path_length: LGTM; single source of truth used elsewhereValues match the arrays (6, 7, 9). Once you update is_path_completed_safe to reuse this, duplication is gone.
370-376: is_valid_path: LGTMStraightforward and explicit. If path count grows, consider switching to
Self::get_path_length(path_id).is_some()to avoid updating in two places.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
contract/src/systems/game.cairo(5 hunks)
🔇 Additional comments (2)
contract/src/systems/game.cairo (2)
351-358: get_step_from_array: LGTMBounds-checked read avoids any out-of-bounds panics while using Span::at safely.
378-384: is_valid_index: LGTMProperly leverages get_path_length; safe and non-panicking.
| // Result type for path operations | ||
| #[derive(Copy, Drop, Serde)] | ||
| pub enum PathResult { | ||
| Success: (u32, u32), | ||
| InvalidPath, | ||
| IndexOutOfBounds, | ||
| PathCompleted, | ||
| } | ||
|
|
||
| #[generate_trait] | ||
| pub impl PathSystemImpl of PathSystemTrait { | ||
| /// Returns the (x, y) coordinates for the given path_id and step index | ||
| fn get_path_step(path_id: u64, index: u32) -> (u32, u32) { | ||
| // Predefined paths | ||
| fn get_path_step(path_id: u64, index: u32) -> Option<(u32, u32)> { | ||
| // Validate path_id first | ||
| let path_data = Self::get_path_data(path_id); | ||
| match path_data { | ||
| Option::None => Option::None, | ||
| Option::Some(steps) => Self::get_step_from_array(steps, index), | ||
| } | ||
| } | ||
|
|
||
| /// Returns the (x, y) coordinates for the given path_id and step index with detailed result | ||
| fn get_path_step_result(path_id: u64, index: u32) -> PathResult { | ||
| // Validate path_id first | ||
| let path_data = Self::get_path_data(path_id); | ||
| match path_data { | ||
| Option::None => PathResult::InvalidPath, | ||
| Option::Some(steps) => { | ||
| match Self::get_step_from_array(steps, index) { | ||
| Option::None => PathResult::IndexOutOfBounds, | ||
| Option::Some(coords) => PathResult::Success(coords), | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add/verify tests for edge cases and legacy API behavior
Given the high-severity bug being addressed, please ensure tests cover:
- Invalid path_id for all public helpers (expect None/InvalidPath, no panics).
- Index exactly at length (expect PathCompleted from get_path_step_result, no movement in advance_enemy_position).
- Index > length (expect None/IndexOutOfBounds, no movement).
- Last valid step followed by call to advance (enemy stays put; path considered completed).
- Legacy is_path_completed no longer panics (after the suggested change), and callers are updated accordingly.
I can draft these tests if helpful.
Run this script to find potential call-site breakages due to get_path_step signature change:
Also applies to: 248-276, 278-294, 295-301, 351-358
🏁 Script executed:
#!/bin/bash
# Look for calls expecting a tuple instead of Option
rg -nP -C2 '\bget_path_step\s*\('
# Ensure no panics remain in the path system APIs
rg -nP -C2 '\bpanic!\(|\bassert\s*\(' contract/src/systems/game.cairoLength of output: 4227
🏁 Script executed:
#!/bin/bash
# Search for get_path_step_result in tests
rg -nP -C2 'get_path_step_result' contract/src/tests || echo "No matches for get_path_step_result"
# Search for is_path_completed in tests
rg -nP -C2 'is_path_completed' contract/src/tests || echo "No matches for is_path_completed"Length of output: 3349
Critical fixes required: update tests and align legacy API behavior
- Tests in
contract/src/tests/test_path_system.cairomust be updated to match the newget_path_stepsignature and behavior:- test_get_path_step_valid_paths
- Change from
let (x, y) = PathSystemImpl::get_path_step(...)tolet coords = PathSystemImpl::get_path_step(...); assert!(coords == Option::Some((expected_x, expected_y)), "…");
- Change from
- test_get_path_step_invalid_path and test_get_path_step_index_out_of_bounds
- Remove
#[should_panic]and change toassert_eq!(PathSystemImpl::get_path_step(invalid_path, idx), Option::None);
- Remove
- test_get_path_step_valid_paths
- Add new tests for
get_path_step_resultcovering:InvalidPathwhenpath_idnot foundIndexOutOfBoundswhenindex> last stepPathCompletedwhenindex== lengthSuccess((x, y))for valid steps
- In
contract/src/systems/game.cairo, the oldis_path_completedstill panics on invalidpath_id. Per the original suggestion, it should return a safe result (e.g.,falseorOption<bool>) and callers updated; either remove thepanic!or wrap it in a safe variant, then adjust or remove the#[should_panic]test accordingly.
I can draft the updated tests and refactoring if that would help.
🤖 Prompt for AI Agents
In contract/src/systems/game.cairo around lines 212 to 247 the new get_path_step
signature returns Option<(u32,u32)> and get_path_step_result returns PathResult,
but tests and the legacy is_path_completed behavior weren't updated; update
tests in contract/src/tests/test_path_system.cairo to: replace destructuring let
(x,y) = PathSystemImpl::get_path_step(...) with let coords =
PathSystemImpl::get_path_step(...); assert that coords ==
Option::Some((expected_x, expected_y)) for valid cases, remove #[should_panic]
on invalid-path and out-of-bounds tests and
assert_eq!(PathSystemImpl::get_path_step(invalid_path, idx), Option::None), add
new tests for get_path_step_result asserting InvalidPath, IndexOutOfBounds,
PathCompleted (when index == length) and Success((x,y)) for valid indices; also
change is_path_completed implementation to not panic on missing path (return a
safe value or Option<bool>) and update any callers/tests to use the safe variant
or adjust assertions accordingly.
| /// Legacy function for backward compatibility - will panic if path completed | ||
| fn is_path_completed(path_id: u64, index: u32) -> bool { | ||
| match Self::is_path_completed_safe(path_id, index) { | ||
| Option::None => panic!("Invalid path_id"), | ||
| Option::Some(result) => result, | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Legacy is_path_completed still panics; replace with graceful behavior
Keeping a panicking branch reintroduces the class of failure we’re trying to eliminate. Prefer a safe default instead of panic, or deprecate this API.
Apply this diff:
- /// Legacy function for backward compatibility - will panic if path completed
+ /// Legacy function for backward compatibility - now graceful on invalid path_id
fn is_path_completed(path_id: u64, index: u32) -> bool {
- match Self::is_path_completed_safe(path_id, index) {
- Option::None => panic!("Invalid path_id"),
- Option::Some(result) => result,
- }
+ match Self::is_path_completed_safe(path_id, index) {
+ Option::Some(result) => result,
+ // Safer default: treat invalid paths as completed to prevent further advancement.
+ Option::None => true,
+ }
}Please run the verification script below to ensure no external callers rely on the panic behavior:
🏁 Script executed:
#!/bin/bash
# Find all call sites of is_path_completed across the repo
rg -nP -C2 '\bis_path_completed\s*\('Length of output: 3425
Legacy is_path_completed behavior change requires test updates
The proposed change to return a safe default (true) instead of panicking on invalid path_id will break existing tests that expect a panic. You’ll need to update the test suite alongside this API change:
• In contract/src/systems/game.cairo (lines ~295–301), you’ve replaced
Option::None => panic!("Invalid path_id"),with
Option::None => true,– this alters the behavior on invalid inputs.
• In contract/src/tests/test_path_system.cairo (lines 91–95), the #[should_panic] test for an invalid path_id must be revised:
– Remove the #[should_panic] attribute.
– Change the test to assert that the function returns true for an invalid path. For example:
- #[should_panic]
- fn test_is_path_completed_invalid_path() {
- let _ = PathSystemImpl::is_path_completed(99_u64, 0_u32);
- }
+ #[test]
+ fn test_is_path_completed_invalid_path_returns_true() {
+ assert!(
+ PathSystemImpl::is_path_completed(99_u64, 0_u32),
+ "Invalid path IDs should be treated as completed"
+ );
+ }Please apply these updates to ensure test coverage aligns with the new, non-panic behavior.
🤖 Prompt for AI Agents
In contract/src/systems/game.cairo around lines 295–301 you changed Option::None
handling from panic!("Invalid path_id") to returning true, which changes
behavior for invalid path_id inputs; update the tests in
contract/src/tests/test_path_system.cairo (around lines 91–95) by removing the
#[should_panic] attribute and replacing the expectation with an assertion that
is_path_completed(invalid_path_id, index) returns true, and scan the test suite
for any other tests expecting a panic on invalid path_id and convert them to
assert the new true default instead.
|
@sotoJ24 pleas fix the tests |
On it |
Submitting changes soon |
|
Done |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contract/src/tests/test_path_system.cairo (1)
144-154: Remove panic expectation; advance on dead enemy should be a no-opTo fully eliminate panic paths, advancing a dead enemy should not panic and should leave the enemy unchanged. Update the test accordingly (and see paired code change in advance_enemy_position).
Apply this diff:
- #[test] - #[should_panic] - fn test_advance_dead_enemy_panics() { + #[test] + fn test_advance_dead_enemy_noop() { let mut enemy = enemy_ok(); // Kill the enemy enemy = EnemySystem::take_damage(@enemy, 100_u32); assert!(!enemy.is_alive, "Enemy should be dead"); - // Try to advance dead enemy - let _ = PathSystemImpl::advance_enemy_position(ref enemy, 0_u64, 0_u32); + // Advancing a dead enemy should be a no-op (no panic, no movement) + let result = PathSystemImpl::advance_enemy_position(ref enemy, 0_u64, 0_u32); + assert!(result.x == enemy.x && result.y == enemy.y && !result.is_alive, "Dead enemy should not move"); }I can also add tests for get_path_step_result covering Success, PathCompleted, IndexOutOfBounds, and InvalidPath.
🧹 Nitpick comments (5)
contract/src/systems/game.cairo (5)
212-219: PathResult::PathCompleted is defined but never returnedCurrently unused by get_path_step_result; consider distinguishing “index == length” from “index > length”.
278-293: Avoid duplicating path lengths; derive from path dataSingle source of truth prevents drift if paths change.
Apply this diff:
- fn is_path_completed_safe(path_id: u64, index: u32) -> Option<bool> { - // Get the path length for each predefined path - let path_length = match path_id { - 0 => Option::Some(6_u32), // Path 0 has 6 steps (indices 0-5) - 1 => Option::Some(7_u32), // Path 1 has 7 steps (indices 0-6) - 2 => Option::Some(9_u32), // Path 2 has 9 steps (indices 0-8) - _ => Option::None, - }; - - match path_length { - Option::None => Option::None, - Option::Some(length) => Option::Some(index >= length), - } - } + fn is_path_completed_safe(path_id: u64, index: u32) -> Option<bool> { + match Self::get_path_data(path_id) { + Option::None => Option::None, + Option::Some(steps) => { + let length: u32 = steps.len().try_into().unwrap(); + Option::Some(index >= length) + } + } + }
295-301: Update outdated doc comment for is_path_completedThe function no longer panics on invalid path_id; reflect the new behavior.
Apply this diff:
- /// Legacy function for backward compatibility - will panic if path completed + /// Legacy function for backward compatibility - graceful on invalid path_id (returns true) fn is_path_completed(path_id: u64, index: u32) -> bool {
360-368: Derive path length from data instead of hardcodingRemoves duplication and keeps length consistent with get_path_data.
Apply this diff:
- fn get_path_length(path_id: u64) -> Option<u32> { - match path_id { - 0 => Option::Some(6_u32), - 1 => Option::Some(7_u32), - 2 => Option::Some(9_u32), - _ => Option::None, - } - } + fn get_path_length(path_id: u64) -> Option<u32> { + match Self::get_path_data(path_id) { + Option::Some(steps) => Option::Some(steps.len().try_into().unwrap()), + Option::None => Option::None, + } + }
370-376: Use get_path_data to validate path IDsAvoids maintaining a separate list.
Apply this diff:
- fn is_valid_path(path_id: u64) -> bool { - match path_id { - 0 | 1 | 2 => true, - _ => false, - } - } + fn is_valid_path(path_id: u64) -> bool { + match Self::get_path_data(path_id) { + Option::Some(_) => true, + Option::None => false, + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
contract/src/systems/game.cairo(5 hunks)contract/src/tests/test_path_system.cairo(2 hunks)
🔇 Additional comments (8)
contract/src/tests/test_path_system.cairo (3)
18-35: LGTM: valid path step assertions align with path dataThe expected coordinates match the predefined paths.
85-90: LGTM: legacy is_path_completed behavior updated in testsTreating invalid path IDs as completed avoids panics downstream.
160-175: LGTM: zigzag path traversal assertions match path 2Checks are consistent with the defined zigzag sequence.
contract/src/systems/game.cairo (5)
224-231: LGTM: get_path_step safely validates path and indexOption-based flow removes the OOB panic surface.
317-317: LGTM: path data exposed via OptionConstructors and spans look correct.
Also applies to: 330-330, 345-345
351-358: LGTM: safe array access with bounds checkPrevents OOB before steps.at(...).
378-384: LGTM: index validation tied to computed lengthSimple and correct.
212-301: All panic!/assert calls removed from the path subsystem andPathCompletedis correctly referenced inget_path_step_result.
| assert_eq!(PathSystemImpl::get_path_step(invalid_path, idx), Option::None); | ||
| } |
There was a problem hiding this comment.
Fix undefined variables in test (invalid_path, idx)
This test won’t compile; use concrete literals or define the variables.
Apply this diff:
- assert_eq!(PathSystemImpl::get_path_step(invalid_path, idx), Option::None);
+ assert_eq!(PathSystemImpl::get_path_step(99_u64, 0_u32), Option::None);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert_eq!(PathSystemImpl::get_path_step(invalid_path, idx), Option::None); | |
| } | |
| assert_eq!(PathSystemImpl::get_path_step(99_u64, 0_u32), Option::None); | |
| } |
🤖 Prompt for AI Agents
In contract/src/tests/test_path_system.cairo around lines 39 to 40, the test
references undefined variables invalid_path and idx which causes a compile
error; replace them with concrete literals or add local let bindings before use
(e.g., define invalid_path with an appropriate Path value and idx with a
usize/u128 literal) and then call PathSystemImpl::get_path_step(invalid_path,
idx) asserting Option::None; ensure the types match the function signature and
the test imports any needed constructors or constants.
| assert_eq!(PathSystemImpl::get_path_step(invalid_path, idx), Option::None); | ||
| } |
There was a problem hiding this comment.
Correct the “index out of bounds” test to use a valid path with an OOB index
Currently reuses the invalid path variables; assert OOB on a valid path.
Apply this diff:
- assert_eq!(PathSystemImpl::get_path_step(invalid_path, idx), Option::None);
+ // Path 0 has length 6 (valid indices 0..=5); 6 is out of bounds
+ assert_eq!(PathSystemImpl::get_path_step(0_u64, 6_u32), Option::None);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert_eq!(PathSystemImpl::get_path_step(invalid_path, idx), Option::None); | |
| } | |
| // Path 0 has length 6 (valid indices 0..=5); 6 is out of bounds | |
| assert_eq!(PathSystemImpl::get_path_step(0_u64, 6_u32), Option::None); |
🤖 Prompt for AI Agents
In contract/src/tests/test_path_system.cairo around lines 44 to 45, the test for
"index out of bounds" incorrectly reuses the invalid_path variable; update the
assertion to call PathSystemImpl::get_path_step with a valid path (e.g.,
valid_path) and an index that is out of bounds for that valid path (e.g., its
length or greater) so the test asserts Option::None on a valid path with an OOB
index.
|
@sotoJ24 you still having a problems with the tests |
On it |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
contract/src/systems/game.cairo (1)
246-259: Differentiate PathCompleted from IndexOutOfBounds in get_path_step_result.Currently, index == length collapses into IndexOutOfBounds. Return PathCompleted for semantic precision and simpler callers.
- fn get_path_step_result(path_id: u64, index: u32) -> PathResult { - // Validate path_id first - let path_data = Self::get_path_data(path_id); - match path_data { - Option::None => PathResult::InvalidPath, - Option::Some(steps) => { - match Self::get_step_from_array(steps, index) { - Option::None => PathResult::IndexOutOfBounds, - Option::Some(coords) => PathResult::Success(coords), - } - } - } - } + fn get_path_step_result(path_id: u64, index: u32) -> PathResult { + match Self::get_path_data(path_id) { + Option::None => PathResult::InvalidPath, + Option::Some(steps) => { + let len: usize = steps.len(); + let i: usize = index.into(); + if i < len { + PathResult::Success(*steps.at(i)) + } else if i == len { + PathResult::PathCompleted + } else { + PathResult::IndexOutOfBounds + } + } + } + }
🧹 Nitpick comments (6)
contract/src/systems/game.cairo (6)
225-233: Good error taxonomy with PathResult.Clear variants; matches stated objectives. Consider adding an event mapping later if you plan to emit diagnostics per result.
291-306: Doc clarity: define “completed” precisely.You return Some(index >= length). Add a brief note: “Completed when index >= length; last valid step is length - 1.” Helps align with get_path_step_result semantics.
316-362: Avoid reallocating path arrays on every call.These paths are static; repeated array construction increases gas. Consider a single source of truth (e.g., one helper returning a Span per id that reuses data) or memoizing in a const-like pattern if feasible in Cairo 1.
373-381: get_path_length: OK, mirrors data.Keep this in sync with get_path_data; consider deriving from the data to avoid duplication.
383-389: is_valid_path duplicates length mapping.Minor DRY win: implement via get_path_length(path_id).is_some().
- fn is_valid_path(path_id: u64) -> bool { - match path_id { - 0 | 1 | 2 => true, - _ => false, - } - } + fn is_valid_path(path_id: u64) -> bool { + matches!(Self::get_path_length(path_id), Option::Some(_)) + }
308-314: Update is_path_completed doc to reflect new behavior
The comment still states “will panic,” but the implementation now returns true for invalid paths instead of panicking. Update the doc as shown below; no callers or tests rely on panics.-/// Legacy function for backward compatibility - will panic if path completed +/// Legacy function for backward compatibility - now graceful: +/// returns true if the path is completed or the path_id is invalid (no panics). fn is_path_completed(path_id: u64, index: u32) -> bool {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contract/src/systems/game.cairo(5 hunks)
🔇 Additional comments (3)
contract/src/systems/game.cairo (3)
237-244: get_path_step: OK.Non-panicking Option API with prior path validation is exactly what we want.
391-397: is_valid_index: LGTM.Leverages get_path_length; aligns with non-panicking contract.
225-306: Verify no panics/asserts remain in PathSystemImpl; add tests for PathCompleted and IndexOutOfBounds
- Grep for
panic!orassert(incontract/src/systems/game.cairolines 225–371 and remove any occurrences within thePathSystemImplblock.- Add or update unit tests under
contract/src/tests/…to explicitly cover both:
PathResult::PathCompleted(whenindex == path_length)PathResult::IndexOutOfBounds(whenindex > path_length)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
contract/src/systems/game.cairo (4)
261-279: Advance logic looks safe and non-panickingAlive-guard added; length-based completion check avoids overflow; next_index computed only when safe. Matches earlier guidance.
298-304: Update docstring to reflect graceful behavior on invalid path_idComment still mentions panic. Align the documentation with current behavior (returns true on invalid path).
Apply:
- /// Legacy function for backward compatibility - will panic if path completed + /// Legacy function for backward compatibility - graceful on invalid path_id (returns true) fn is_path_completed(path_id: u64, index: u32) -> bool { match Self::is_path_completed_safe(path_id, index) { Option::None => true, Option::Some(result) => result, } }
353-361: Bounds-safe indexing implemented correctlySingle usize conversion used for both check and access eliminates TOCTOU/mismatch risks.
246-259: Differentiate PathCompleted (index == length) vs IndexOutOfBounds (index > length)Currently, both collapse to IndexOutOfBounds via get_step_from_array. Return PathCompleted when index == length for precise control flow and parity with is_path_completed_safe.
Apply:
- fn get_path_step_result(path_id: u64, index: u32) -> PathResult { - // Validate path_id first - let path_data = Self::get_path_data(path_id); - match path_data { - Option::None => PathResult::InvalidPath, - Option::Some(steps) => { - match Self::get_step_from_array(steps, index) { - Option::None => PathResult::IndexOutOfBounds, - Option::Some(coords) => PathResult::Success(coords), - } - } - } - } + fn get_path_step_result(path_id: u64, index: u32) -> PathResult { + match Self::get_path_data(path_id) { + Option::None => PathResult::InvalidPath, + Option::Some(steps) => { + let len_u: usize = steps.len(); + let len: u32 = len_u.try_into().unwrap(); + if index < len { + PathResult::Success(*steps.at(index.into())) + } else if index == len { + PathResult::PathCompleted + } else { + PathResult::IndexOutOfBounds + } + } + } + }
🧹 Nitpick comments (2)
contract/src/systems/game.cairo (2)
281-296: Avoid duplicated path lengths; reuse get_path_length()Hard-coding lengths here risks drift from get_path_length. Delegate to it instead.
Apply:
- fn is_path_completed_safe(path_id: u64, index: u32) -> Option<bool> { - // Get the path length for each predefined path - let path_length = match path_id { - 0 => Option::Some(6_u32), // Path 0 has 6 steps (indices 0-5) - 1 => Option::Some(7_u32), // Path 1 has 7 steps (indices 0-6) - 2 => Option::Some(9_u32), // Path 2 has 9 steps (indices 0-8) - _ => Option::None, - }; - - match path_length { - Option::None => Option::None, - Option::Some(length) => Option::Some(index >= length), - } - } + fn is_path_completed_safe(path_id: u64, index: u32) -> Option<bool> { + match Self::get_path_length(path_id) { + Option::None => Option::None, + Option::Some(length) => Option::Some(index >= length), + } + }
363-371: OK: Centralized path lengths; consider a test to guard against driftConstants match the arrays above. Add a test asserting get_path_length(id) equals get_path_data(id).unwrap().len() for each id.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contract/src/systems/game.cairo(5 hunks)
🔇 Additional comments (5)
contract/src/systems/game.cairo (5)
225-233: Good: Clear non-panicking result enum introducedPathResult variants cover the needed outcomes and align with the non-panicking goal.
320-320: LGTM: Path data returned via Span for predefined pathsReturning spans is consistent and avoids copying.
Also applies to: 333-333, 348-348
373-379: LGTM: is_valid_path is simple and fastNo further action.
381-387: LGTM: is_valid_index delegates to get_path_lengthThis keeps checks consistent across APIs.
237-244: No tuple destructuring of get_path_step found; API shift verified
All call sites now handle the Option return.
2 participants
📋 Description
Fixes a critical security vulnerability in the path system that could lead to contract panics and DoS attacks. The
get_path_step()andadvance_enemy_position()functions were usingpanic!()andassert()for input validation, making the game vulnerable to crashes from malicious or malformed inputs.Closes #176
🔧 Changes Made
Security Improvements
Result<T, PathError>Code Quality
📁 Files Modified
contract/src/systems/game.cairo- Core path system implementation🧪 Testing
✅ Checklist
📈 Performance Impact
Minimal overhead - Error checking adds negligible gas costs while preventing catastrophic failures.
🔍 Review Focus Areas
Summary by CodeRabbit
New Features
Bug Fixes
Tests